From: Keir Fraser Date: Thu, 19 Jun 2008 10:09:10 +0000 (+0100) Subject: vmx: Clean up and fix guest MSR load/save handling: X-Git-Tag: archive/raspbian/4.8.0-1+rpi1~1^2~14192^2~46 X-Git-Url: https://dgit.raspbian.org/%22http:/www.example.com/cgi/%22https:/%22bookmarks://%22Dat/%22http:/www.example.com/cgi/%22https:/%22bookmarks:/%22Dat?a=commitdiff_plain;h=c385f786c2b062e79c92a760ca0973ef0d54371a;p=xen.git vmx: Clean up and fix guest MSR load/save handling: 1. msr_bitmap/msr_area/msr_host_area must be freed when a vcpu is destroyed 2. vmx_create_vmcs()/vmx_destroy_vmcs() are only ever called once, and can hence be simplified slightly 3. Change vmx_*_msr() interfaces to make it crystal clear that they operate only on current (hence safe against vmwrite() and also against concurrency races). 4. Change vmx_add_*_msr() implementation to make it crystal clear that msr_area is not dereferenced before it is allocated. Only (1) is a bug fix. (2)-(4) are for code clarity. Signed-off-by: Keir Fraser --- diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 7056251c9e..ff5ca4e7d4 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -677,10 +677,11 @@ static int construct_vmcs(struct vcpu *v) return 0; } -int vmx_read_guest_msr(struct vcpu *v, u32 msr, u64 *val) +int vmx_read_guest_msr(u32 msr, u64 *val) { - unsigned int i, msr_count = v->arch.hvm_vmx.msr_count; - const struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area; + struct vcpu *curr = current; + unsigned int i, msr_count = curr->arch.hvm_vmx.msr_count; + const struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area; for ( i = 0; i < msr_count; i++ ) { @@ -694,10 +695,11 @@ int vmx_read_guest_msr(struct vcpu *v, u32 msr, u64 *val) return -ESRCH; } -int vmx_write_guest_msr(struct vcpu *v, u32 msr, u64 val) +int vmx_write_guest_msr(u32 msr, u64 val) { - unsigned int i, msr_count = v->arch.hvm_vmx.msr_count; - struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area; + struct vcpu *curr = current; + unsigned int i, msr_count = curr->arch.hvm_vmx.msr_count; + struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area; for ( i = 0; i < msr_count; i++ ) { @@ -711,61 +713,63 @@ int vmx_write_guest_msr(struct vcpu *v, u32 msr, u64 val) return -ESRCH; } -int vmx_add_guest_msr(struct vcpu *v, u32 msr) +int vmx_add_guest_msr(u32 msr) { - unsigned int i, msr_count = v->arch.hvm_vmx.msr_count; - struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area; - - for ( i = 0; i < msr_count; i++ ) - if ( msr_area[i].index == msr ) - return 0; - - if ( msr_count == (PAGE_SIZE / sizeof(struct vmx_msr_entry)) ) - return -ENOSPC; + struct vcpu *curr = current; + unsigned int i, msr_count = curr->arch.hvm_vmx.msr_count; + struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area; if ( msr_area == NULL ) { if ( (msr_area = alloc_xenheap_page()) == NULL ) return -ENOMEM; - v->arch.hvm_vmx.msr_area = msr_area; + curr->arch.hvm_vmx.msr_area = msr_area; __vmwrite(VM_EXIT_MSR_STORE_ADDR, virt_to_maddr(msr_area)); __vmwrite(VM_ENTRY_MSR_LOAD_ADDR, virt_to_maddr(msr_area)); } + for ( i = 0; i < msr_count; i++ ) + if ( msr_area[i].index == msr ) + return 0; + + if ( msr_count == (PAGE_SIZE / sizeof(struct vmx_msr_entry)) ) + return -ENOSPC; + msr_area[msr_count].index = msr; msr_area[msr_count].mbz = 0; msr_area[msr_count].data = 0; - v->arch.hvm_vmx.msr_count = ++msr_count; + curr->arch.hvm_vmx.msr_count = ++msr_count; __vmwrite(VM_EXIT_MSR_STORE_COUNT, msr_count); __vmwrite(VM_ENTRY_MSR_LOAD_COUNT, msr_count); return 0; } -int vmx_add_host_load_msr(struct vcpu *v, u32 msr) +int vmx_add_host_load_msr(u32 msr) { - unsigned int i, msr_count = v->arch.hvm_vmx.host_msr_count; - struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.host_msr_area; - - for ( i = 0; i < msr_count; i++ ) - if ( msr_area[i].index == msr ) - return 0; - - if ( msr_count == (PAGE_SIZE / sizeof(struct vmx_msr_entry)) ) - return -ENOSPC; + struct vcpu *curr = current; + unsigned int i, msr_count = curr->arch.hvm_vmx.host_msr_count; + struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.host_msr_area; if ( msr_area == NULL ) { if ( (msr_area = alloc_xenheap_page()) == NULL ) return -ENOMEM; - v->arch.hvm_vmx.host_msr_area = msr_area; + curr->arch.hvm_vmx.host_msr_area = msr_area; __vmwrite(VM_EXIT_MSR_LOAD_ADDR, virt_to_maddr(msr_area)); } + for ( i = 0; i < msr_count; i++ ) + if ( msr_area[i].index == msr ) + return 0; + + if ( msr_count == (PAGE_SIZE / sizeof(struct vmx_msr_entry)) ) + return -ENOSPC; + msr_area[msr_count].index = msr; msr_area[msr_count].mbz = 0; rdmsrl(msr, msr_area[msr_count].data); - v->arch.hvm_vmx.host_msr_count = ++msr_count; + curr->arch.hvm_vmx.host_msr_count = ++msr_count; __vmwrite(VM_EXIT_MSR_LOAD_COUNT, msr_count); return 0; @@ -776,21 +780,17 @@ int vmx_create_vmcs(struct vcpu *v) struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx; int rc; - if ( arch_vmx->vmcs == NULL ) - { - if ( (arch_vmx->vmcs = vmx_alloc_vmcs()) == NULL ) - return -ENOMEM; + if ( (arch_vmx->vmcs = vmx_alloc_vmcs()) == NULL ) + return -ENOMEM; - INIT_LIST_HEAD(&arch_vmx->active_list); - __vmpclear(virt_to_maddr(arch_vmx->vmcs)); - arch_vmx->active_cpu = -1; - arch_vmx->launched = 0; - } + INIT_LIST_HEAD(&arch_vmx->active_list); + __vmpclear(virt_to_maddr(arch_vmx->vmcs)); + arch_vmx->active_cpu = -1; + arch_vmx->launched = 0; if ( (rc = construct_vmcs(v)) != 0 ) { vmx_free_vmcs(arch_vmx->vmcs); - arch_vmx->vmcs = NULL; return rc; } @@ -801,13 +801,13 @@ void vmx_destroy_vmcs(struct vcpu *v) { struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx; - if ( arch_vmx->vmcs == NULL ) - return; - vmx_clear_vmcs(v); vmx_free_vmcs(arch_vmx->vmcs); - arch_vmx->vmcs = NULL; + + free_xenheap_page(v->arch.hvm_vmx.host_msr_area); + free_xenheap_page(v->arch.hvm_vmx.msr_area); + free_xenheap_page(v->arch.hvm_vmx.msr_bitmap); } void vm_launch_fail(void) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 16dfe29e10..5f048ab550 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1655,7 +1655,7 @@ static int vmx_msr_read_intercept(struct cpu_user_regs *regs) goto done; } - if ( vmx_read_guest_msr(v, ecx, &msr_content) == 0 ) + if ( vmx_read_guest_msr(ecx, &msr_content) == 0 ) break; if ( is_last_branch_msr(ecx) ) @@ -1817,12 +1817,12 @@ static int vmx_msr_write_intercept(struct cpu_user_regs *regs) for ( ; (rc == 0) && lbr->count; lbr++ ) for ( i = 0; (rc == 0) && (i < lbr->count); i++ ) - if ( (rc = vmx_add_guest_msr(v, lbr->base + i)) == 0 ) + if ( (rc = vmx_add_guest_msr(lbr->base + i)) == 0 ) vmx_disable_intercept_for_msr(v, lbr->base + i); } if ( (rc < 0) || - (vmx_add_host_load_msr(v, ecx) < 0) ) + (vmx_add_host_load_msr(ecx) < 0) ) vmx_inject_hw_exception(v, TRAP_machine_check, 0); else { @@ -1842,7 +1842,7 @@ static int vmx_msr_write_intercept(struct cpu_user_regs *regs) switch ( long_mode_do_msr_write(regs) ) { case HNDL_unhandled: - if ( (vmx_write_guest_msr(v, ecx, msr_content) != 0) && + if ( (vmx_write_guest_msr(ecx, msr_content) != 0) && !is_last_branch_msr(ecx) ) wrmsr_hypervisor_regs(ecx, regs->eax, regs->edx); break; diff --git a/xen/arch/x86/hvm/vmx/vpmu_core2.c b/xen/arch/x86/hvm/vmx/vpmu_core2.c index 5539874774..5062d9c28d 100644 --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c @@ -219,12 +219,12 @@ static int core2_vpmu_alloc_resource(struct vcpu *v) return 0; wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0); - if ( vmx_add_host_load_msr(v, MSR_CORE_PERF_GLOBAL_CTRL) ) + if ( vmx_add_host_load_msr(MSR_CORE_PERF_GLOBAL_CTRL) ) return 0; - if ( vmx_add_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL) ) + if ( vmx_add_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL) ) return 0; - vmx_write_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL, -1ULL); + vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, -1ULL); pmu_enable = xmalloc_bytes(sizeof(struct core2_pmu_enable) + (core2_get_pmc_count()-1)*sizeof(char)); @@ -347,7 +347,7 @@ static int core2_vpmu_do_wrmsr(struct cpu_user_regs *regs) break; case MSR_CORE_PERF_FIXED_CTR_CTRL: non_global_ctrl = msr_content; - vmx_read_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL, &global_ctrl); + vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, &global_ctrl); global_ctrl >>= 32; for ( i = 0; i < 3; i++ ) { @@ -359,7 +359,7 @@ static int core2_vpmu_do_wrmsr(struct cpu_user_regs *regs) break; default: tmp = ecx - MSR_P6_EVNTSEL0; - vmx_read_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL, &global_ctrl); + vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, &global_ctrl); if ( tmp >= 0 && tmp < core2_get_pmc_count() ) core2_vpmu_cxt->pmu_enable->arch_pmc_enable[tmp] = (global_ctrl >> tmp) & (msr_content >> 22) & 1; @@ -385,7 +385,7 @@ static int core2_vpmu_do_wrmsr(struct cpu_user_regs *regs) if ( type != MSR_TYPE_GLOBAL ) wrmsrl(ecx, msr_content); else - vmx_write_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL, msr_content); + vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, msr_content); return 1; } @@ -410,7 +410,7 @@ static int core2_vpmu_do_rdmsr(struct cpu_user_regs *regs) msr_content = core2_vpmu_cxt->global_ovf_status; break; case MSR_CORE_PERF_GLOBAL_CTRL: - vmx_read_guest_msr(v, MSR_CORE_PERF_GLOBAL_CTRL, &msr_content); + vmx_read_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, &msr_content); break; default: rdmsrl(regs->ecx, msr_content); diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h index 4ac9b43e4f..9d900996cb 100644 --- a/xen/include/asm-x86/hvm/vmx/vmcs.h +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h @@ -333,10 +333,10 @@ enum vmcs_field { #define VMCS_VPID_WIDTH 16 void vmx_disable_intercept_for_msr(struct vcpu *v, u32 msr); -int vmx_read_guest_msr(struct vcpu *v, u32 msr, u64 *val); -int vmx_write_guest_msr(struct vcpu *v, u32 msr, u64 val); -int vmx_add_guest_msr(struct vcpu *v, u32 msr); -int vmx_add_host_load_msr(struct vcpu *v, u32 msr); +int vmx_read_guest_msr(u32 msr, u64 *val); +int vmx_write_guest_msr(u32 msr, u64 val); +int vmx_add_guest_msr(u32 msr); +int vmx_add_host_load_msr(u32 msr); #endif /* ASM_X86_HVM_VMX_VMCS_H__ */